-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make boot.sh process PID 1 #53
base: master
Are you sure you want to change the base?
Conversation
we want boot.sh to correctly receive and forwards signals. this commit removes the double indirection (`CMD` in non array form will already execute the command in a sh subshell and we wrapped our script in another one).
Hi, thanks for the PR. We've merged master into this one but it seems this only makes it restart faster on development mode as is. We have added support for a callback function on exit and a default implementation which exits the node process. This makes booting faster but we're not entirely certain about the proposed API. Even without this API I guess it's just a faster random exit. We have pushed this experimental feature on feature/faster-stop but I'm not sure if/how you can adapt the PR to use that branch instead. Food for thoughts, suggestions welcome. |
from reading the docs I think the default should be:
this should allow express to shutdown gracefully (e.g. stop accepting new requests, but still respond to ongoing requests). |
We only exported setExitHandler but that doesn't do much when we don't import it first.
Here is a new exitHandler based on @nvdk's suggestions and research. It doesn't fully behave the way we intend (the last message is not logged) but does further serve as a proof of concept.
I checked node's API but better to use Express's API indeed. We could supply the server to the shutdown function which would let users call server.close( ... ) or whatever else. Trying to execute this I did ran into a some issues: the callback function supplied to server.close did not seem to execute (or at least, it can't log anything). Basing myself on the documentation instead of the post, I find app.listen which returns http.server. The docs regarding the close function further point to server.close for the close function. All of this points to what you suggest, though running the code I've just pushed onto your branch, This should not be a show-stopper but perhaps there's something we have misunderstood? I have not tried with |
small addition to #51 which should be merged first, this makes boot.sh process 0 in the container so that it correctly receives all signals sent to it. this allows it to correctly respond to kill signals sent from docker